Skip to content

refactor(datacell): extract variable record layouts - #2737

Open
LHT129 wants to merge 1 commit into
antgroup:mainfrom
LHT129:2026-08-21-剥离-byte-range-layout-与-variable-record-layout

Hidden character warning

The head ref may contain hidden characters: "2026-08-21-\u5265\u79bb-byte-range-layout-\u4e0e-variable-record-layout"
Open

refactor(datacell): extract variable record layouts#2737
LHT129 wants to merge 1 commit into
antgroup:mainfrom
LHT129:2026-08-21-剥离-byte-range-layout-与-variable-record-layout

Conversation

@LHT129

@LHT129 LHT129 commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

What

  • Add ByteRangeLayout for explicit (offset, length) access to opaque bytes.
  • Add policy-based VariableRecordLayout composed from FixedLayout and ByteRangeLayout.
  • Migrate SparseVectorDataCell while preserving the V2 and legacy location schemas.
  • Migrate MultiVectorDataCell while preserving its uint64_t offset table and serialized byte order.

Design boundary

Layout owns addressing, byte organization, IO delegation, append allocation, and serialization primitives. It does not interpret quantized codes or perform distance computation.

Sparse query range merging remains in the DataCell because it coordinates result ordering and computation.

Tests

  • Focused unit tests: 17 cases and 14,292 assertions passed.
  • Debug build passed.
  • Release build passed.

Copilot AI lite review requested due to automatic review settings August 21, 2026 06:38
@LHT129 LHT129 added kind/improvement Optimizations, UX polish, or minor improvements 性能优化、体验打磨或细节改良 version/1.1 labels Aug 21, 2026
@vsag-bot

Copy link
Copy Markdown
Collaborator

/label status/waiting-for-review
/waiting-on reviewer
/request-review @jiaweizone
/request-review @wxyucs
/request-review @inabao

@mergify

mergify Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Merge Protections

🟢 All 2 merge protections satisfied — ready to merge.

Show 2 satisfied protections

🟢 Require kind label

  • label~=^kind/

🟢 Require version label

  • label~=^version/

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors variable-length record storage in the DataCell layer by introducing reusable layout primitives (explicit byte-range addressing plus policy-based variable record locations) and migrating SparseVectorDataCell / MultiVectorDataCell to use them while preserving existing on-disk schemas.

Changes:

  • Added ByteRangeLayout for explicit (offset, length) access over opaque bytes and IO delegation/serialization.
  • Added VariableRecordLayout composed of FixedLayout (locations) + ByteRangeLayout (payload), with pluggable location policies.
  • Migrated SparseVectorDataCell and MultiVectorDataCell to the new layout abstractions while keeping v2 + legacy serialization compatibility.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/layout/variable_record_layout.h New policy-based variable-record layout (locations + payload) and location policy helpers.
src/layout/variable_record_layout_test.cpp Unit tests validating record append, location mapping, and batched reads.
src/layout/byte_range_layout.h New byte-range wrapper over BasicIO with offset/length read/write + serialization passthrough.
src/layout/byte_range_layout_test.cpp Unit tests for range addressing and serialization round-trip.
src/datacell/sparse_vector_datacell.inl Migrates sparse-vector code storage/query paths to VariableRecordLayout.
src/datacell/sparse_vector_datacell.h Replaces bespoke packed location table + payload IO fields with VariableRecordLayout.
src/datacell/multi_vector_datacell.inl Migrates multi-vector storage/query/serialization to VariableRecordLayout with header-length policy.
src/datacell/multi_vector_datacell.h Replaces offset table + payload/current-offset fields with VariableRecordLayout.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/layout/variable_record_layout.h
Comment thread src/layout/variable_record_layout.h
Comment thread src/datacell/multi_vector_datacell.inl Outdated
Comment thread src/layout/byte_range_layout.h Outdated
Copilot AI review requested due to automatic review settings August 21, 2026 07:22
@LHT129
LHT129 force-pushed the 2026-08-21-剥离-byte-range-layout-与-variable-record-layout branch from cbc490a to 9ec0f1b Compare August 21, 2026 07:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

src/datacell/sparse_vector_datacell.inl:43

  • The load_location lambda declares DocLocation location{} but never uses it, which will trigger an unused-variable warning (and can break builds if warnings are treated as errors).
    const auto load_location = [this](InnerIdType id) {
        DocLocation location{};
        return layout_.ReadLocation(id);
    };

src/datacell/multi_vector_datacell.inl:218

  • This comment still references offset_io_, but the implementation now reads from layout_ (locations table). Updating the comment will avoid confusion during future maintenance.
    // Step 1: Read all offsets (offset_io_ is MemoryBlockIO, in-memory, fast)
    std::vector<uint64_t> offsets(id_count);
    for (InnerIdType i = 0; i < id_count; ++i) {
        offsets[i] = layout_.ReadLocation(idx[i]);

Comment thread src/datacell/multi_vector_datacell.inl Outdated
Comment thread src/datacell/multi_vector_datacell.inl
Signed-off-by: LHT129 <tianlan.lht@antgroup.com>
Assisted-by: Codex:GPT-5
Copilot AI review requested due to automatic review settings August 21, 2026 07:33
@LHT129
LHT129 force-pushed the 2026-08-21-剥离-byte-range-layout-与-variable-record-layout branch from 9ec0f1b to 46d147f Compare August 21, 2026 07:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/datacell/sparse_vector_datacell.inl:43

  • The load_location lambda declares DocLocation location{} but never uses it, which will trigger unused-variable warnings (and can fail builds if warnings are treated as errors). The lambda can just return layout_.ReadLocation(id) directly.
    const auto load_location = [this](InnerIdType id) {
        DocLocation location{};
        return layout_.ReadLocation(id);
    };

src/layout/byte_range_layout.h:75

  • ByteRangeLayout::Prefetch forwards its second argument to BasicIO::Prefetch(offset, cache_line), but the parameter is named length, which misrepresents what the value means. Renaming it to cache_line (and matching the default) makes the API clearer and consistent with BasicIO.
    void
    Prefetch(uint64_t offset, uint64_t length) {
        io_->Prefetch(offset, length);
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/improvement Optimizations, UX polish, or minor improvements 性能优化、体验打磨或细节改良 size/XL version/1.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants